Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add equals/hashCode to avoid coder warning #34036

Merged
merged 6 commits into from
Feb 25, 2025
Merged

add equals/hashCode to avoid coder warning #34036

merged 6 commits into from
Feb 25, 2025

Conversation

scwhittle
Copy link
Contributor

Example warning

WARNING: Can't verify serialized elements of type OffsetHolder have well defined equals method. This may produce incorrect results on some PipelineRunner implementations
    Feb 20, 2025 11:02:40 AM org.apache.beam.sdk.util.MutationDetectors$CodedValueMutationDetector verifyUnmodifiedThrowingCheckedExceptions
    WARNING: Coder of type class org.apache.beam.sdk.coders.KvCoder has a #structuralValue method which does not return true when the encoding of the elements is equal. Element KV{{topic=any, from=1, to=3, delay=0.2}, org.apache.beam.io.debezium.KafkaSourceConsumerFn$OffsetHolder@58983367}
    Feb 20, 2025 11:02:40 AM org.apache.beam.sdk.util.MutationDetectors$CodedValueMutationDetector verifyUnmodifiedThrowingCheckedExceptions

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@github-actions GitHub Actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @ahmedabu98 for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

ahmedabu98
ahmedabu98 reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@ahmedabu98 ahmedabu98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, left a comment

Also wondering if we could set explicit types for OffsetHolder's history and offset fields (out of scope for this PR but it could be an improvement)

sdks/java/io/debezium/src/main/java/org/apache/beam/io/debezium/KafkaSourceConsumerFn.java Outdated
}
OffsetHolder otherOffset = (OffsetHolder) other;
return Objects.equals(offset, otherOffset.offset)
&& Objects.equals(history, otherOffset.history)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that history is a List<?> but in practice below it is a List<byte[]>. Is this not problematic? As it would compare byte array references instead of the content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added logic to do deeper comparison and added a test of equality.

@scwhittle scwhittle requested a review from ahmedabu98 February 24, 2025 13:02
ahmedabu98
ahmedabu98 approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@ahmedabu98 ahmedabu98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

just need to add an import and get spotless to run green

sdks/java/io/debezium/src/test/java/org/apache/beam/io/debezium/KafkaSourceConsumerFnTest.java
map.put("a", 1);
map.put("b", 2);
ArrayList<byte[]> list = new ArrayList<>();
list.add("abc".getBytes(StandardCharsets.US_ASCII));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing an import for StandardCharsets

@scwhittle
Copy link
Contributor Author

remaining spotless error is unrelated

@scwhittle scwhittle merged commit 87f0ed3 into apache:master Feb 25, 2025
15 of 16 checks passed
@scwhittle scwhittle deleted the test_fix branch February 25, 2025 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmedabu98 ahmedabu98 ahmedabu98 approved these changes

Assignees
No one assigned
Labels
io java Next Action: Reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scwhittle @ahmedabu98